Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: De-globalize JSAPI in IrisGrid package #1262

Merged
merged 16 commits into from
May 8, 2023

Conversation

vbabich
Copy link
Collaborator

@vbabich vbabich commented May 4, 2023

  • De-globalize JSAPI in the IrisGrid package.
  • De-globalize JSAPI in DateUtils and TableUtils in thejsapi-utils package.
  • Update dashboard-core-plugins, code-studio, embed-grid` to match the IrisGrid package changes.

BREAKING CHANGE:

  • DateUtils static methods makeDateWrapper, getNextDate , parseDateRange now require the JSAPI object as the first argument.
  • IrisGridUtils static methods dehydrateIrisGridState, hydrateIrisGridState, hydrateQuickFilters, dehydrateAdvancedFilters, hydrateAdvancedFilters, dehydrateAdvancedFilterOptions, hydrateAdvancedFilterOptions, dehydratePendingDataMap, hydratePendingDataMap, dehydrateValue, hydrateValue, dehydrateDateTime, hydrateDateTime, hydrateLong, hydrateSort, applyTableSettings, getFiltersFromInputFilters, rangeSetFromRanges converted to instance methods. Consumers now need to create an IrisGridUtils instance and pass the JSAPI object to the constructor.
  • TableUtils static methods makeQuickFilter, makeQuickFilterFromComponent, makeQuickNumberFilter, makeQuickTextFilter, makeQuickBooleanFilter, makeQuickDateFilter, makeQuickDateFilterWithOperation, makeQuickCharFilter, makeAdvancedFilter, makeAdvancedValueFilter, makeFilterValue, makeFilterRawValue, makeValue, makeSelectValueFilter converted to instance methods. Consumers now need to create a TableUtils instance and pass the JSAPI object to the constructor.
  • IrisGridTableModel, IrisGridTableModelTemplate, IrisGridProxyModel constructors require the JSAPI object in the first argument.
  • IrisGridTestUtils.makeModel, IrisGridModelFactory.makeModel now require the JSAPI object in the first argument.
  • IrisGridContextMenuHandler constructor requires the JSAPI object in the second argument.
  • IrisGridPanel requires a new makeApi prop, a function that resolves with the JSAPI instance.
  • CrossColumnSearch.createSearchFilter requires the JSAPI object argument.
  • Components AdvancedFilterCreatorSelectValue, AdvancedFilterCreatorSelectValueList, ChartBuilder, GotoRow, IrisGrid, IrisGridModelUpdater, IrisGridPartitionSelector, PartitionSelectorSearch, TableCSVExporter, TableSaver, TreeTableViewportUpdater, RowFormatEditor, ColumnFormatEditor, ConditionEditor now require the JSAPI object passed in the new prop dh.
  • Components AdvancedFilterCreator, AdvancedFilterCreatorFilterItem require the TableUtils instance pass in the new prop tableUtils.
  • ConditionalFormattingUtils static methods getFormatColumns, isDateConditionValid require the JSAPI object in the first argument.
  • ConditionalFormattingAPIUtils static method makeRowFormatColumn requires the JSAPI object in the first argument.

@vbabich vbabich self-assigned this May 4, 2023
@vbabich vbabich requested a review from mofojed May 4, 2023 12:57
@vbabich vbabich marked this pull request as ready for review May 4, 2023 12:58
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just a little suggested cleanup.

@@ -251,6 +252,7 @@ export type FilterMap = Map<
>;
export interface IrisGridProps {
children: React.ReactNode;
dh: DhType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note - it would be ideal if everything was abstracted through the model and we didn't have to depend on dh being passed in to IrisGrid. I think that change is much more work than we're looking for right now though with this change.

Also, we could still use the context in the class component to get the API from the context: https://legacy.reactjs.org/docs/context.html#classcontexttype

I don't think that's much better though.

@vbabich vbabich force-pushed the jsapi-deglobalize-irisgrid branch from aff5958 to 72fbc81 Compare May 8, 2023 13:28
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #1262 (5128dbf) into main (87fa2ef) will increase coverage by 0.03%.
The diff coverage is 65.00%.

@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
+ Coverage   45.39%   45.43%   +0.03%     
==========================================
  Files         502      502              
  Lines       34706    34764      +58     
  Branches     8677     8679       +2     
==========================================
+ Hits        15754    15794      +40     
- Misses      18901    18919      +18     
  Partials       51       51              
Flag Coverage Δ
unit 45.43% <65.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/main/AppMainContainer.tsx 34.35% <0.00%> (-0.12%) ⬇️
packages/code-studio/src/main/WidgetUtils.ts 5.55% <0.00%> (ø)
packages/dashboard-core-plugins/src/GridPlugin.tsx 48.00% <0.00%> (-4.18%) ⬇️
...ckages/dashboard-core-plugins/src/PandasPlugin.tsx 50.00% <0.00%> (-2.39%) ⬇️
packages/iris-grid/src/ColumnStatistics.tsx 3.40% <ø> (ø)
packages/iris-grid/src/CrossColumnSearch.tsx 8.64% <ø> (ø)
packages/iris-grid/src/GotoRow.tsx 39.58% <ø> (ø)
packages/iris-grid/src/IrisGridCopyHandler.tsx 82.80% <ø> (ø)
packages/iris-grid/src/IrisGridMetricCalculator.ts 70.58% <ø> (ø)
packages/iris-grid/src/IrisGridModel.ts 31.66% <ø> (ø)
... and 41 more

@vbabich vbabich force-pushed the jsapi-deglobalize-irisgrid branch from 72fbc81 to f1f7f75 Compare May 8, 2023 15:34
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, need to update the Breaking Change bit

Slightly confused how text in the Go To Row has gotten shifted down by a pixel

@vbabich
Copy link
Collaborator Author

vbabich commented May 8, 2023

Updated breaking changes in the first comment.
The 1px shift in the Go To Row doesn't seem to be related, I'm getting the same e2e result on latest main:
117 pixels (ratio 0.01 of all image pixels) are different. I guess no one updated the screenshot in a while.

@vbabich vbabich merged commit 588cb8f into deephaven:main May 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants